-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Web Import: Add New Entries to 'Imported Entries' Group #12998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of |
2732800
to
d9ea011
Compare
@@ -501,4 +504,17 @@ private List<BibEntry> handlePdfUrl(String pdfUrl) throws IOException { | |||
return List.of(); | |||
} | |||
} | |||
|
|||
private void addToImportEntriesGroup(List<BibEntry> entryToInsert) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method name 'entryToInsert' is misleading as it suggests a single entry, but it is actually a list of entries. This can cause confusion and reduce code readability.
@@ -501,4 +504,17 @@ private List<BibEntry> handlePdfUrl(String pdfUrl) throws IOException { | |||
return List.of(); | |||
} | |||
} | |||
|
|||
private void addToImportEntriesGroup(List<BibEntry> entryToInsert) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method 'addToImportEntriesGroup' should not return null. It should use Optional to handle cases where no group is found, ensuring better null safety and code clarity.
@@ -61,6 +62,10 @@ public static String getShortDescriptionAllEntriesGroup() { | |||
return Localization.lang("<b>All Entries</b> (this group cannot be edited or removed)"); | |||
} | |||
|
|||
public static String getShortDescriptionSmartGroup(SmartGroup smartGroup) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method getShortDescriptionSmartGroup returns a String, which could potentially be null. New public methods should not return null and should use java.util.Optional instead.
@@ -61,6 +62,10 @@ public static String getShortDescriptionAllEntriesGroup() { | |||
return Localization.lang("<b>All Entries</b> (this group cannot be edited or removed)"); | |||
} | |||
|
|||
public static String getShortDescriptionSmartGroup(SmartGroup smartGroup) { | |||
return Localization.lang("<b>Smart Group</b> (Import Entries)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string 'Smart Group' should be in sentence case as per the guidelines, so it should be 'Smart group'.
@@ -17,6 +17,7 @@ | |||
import org.jabref.model.groups.GroupTreeNode; | |||
import org.jabref.model.groups.KeywordGroup; | |||
import org.jabref.model.groups.SearchGroup; | |||
import org.jabref.model.groups.SmartGroup; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code uses Java SWING imports, which contradicts the instruction to use only JavaFX for UI technology. This could lead to inconsistencies in the UI framework used.
boolean isGrpExist = parent.getGroupNode() | ||
.getChildren() | ||
.stream() | ||
.map(GroupTreeNode::getGroup) | ||
.anyMatch(grp -> grp instanceof SmartGroup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name isGrpExist does not follow camel-case naming convention. It should be renamed to isGroupExist for consistency.
@@ -47,6 +48,13 @@ void setUp() { | |||
preferences = mock(GuiPreferences.class); | |||
dialogService = mock(DialogService.class, Answers.RETURNS_DEEP_STUBS); | |||
|
|||
when(preferences.getLibraryPreferences()).thenReturn(new LibraryPreferences( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch adds a new mock for LibraryPreferences, but the method name 'getLibraryPreferences' does not follow camel-case naming convention.
@@ -76,6 +78,10 @@ public void initialize() { | |||
downloadLinkedOnlineFiles.selectedProperty().bindBidirectional(viewModel.shouldDownloadLinkedOnlineFiles()); | |||
keepDownloadUrl.selectedProperty().bindBidirectional(viewModel.shouldKeepDownloadUrl()); | |||
|
|||
addImportedEntries.selectedProperty().bindBidirectional(viewModel.getAddImportedEntries()); | |||
addImportedEntriesGroupName.textProperty().bindBidirectional(viewModel.getAddImportedEntriesGroupName()); | |||
addImportedEntriesGroupName.disableProperty().bind(addImportedEntries.selectedProperty().not()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name 'addImportedEntriesGroupName' should be in camel-case format, such as 'addImportedEntriesGroupName'.
@@ -26,6 +27,19 @@ private static String serializeAllEntriesGroup() { | |||
return MetadataSerializationConfiguration.ALL_ENTRIES_GROUP_ID; | |||
} | |||
|
|||
private String serializeSmartGroup(SmartGroup group) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method serializeSmartGroup is private and returns a String. If this method is intended to be used externally, it should return an Optional instead of null, following modern Java practices.
@@ -206,6 +210,24 @@ private static KeywordGroup keywordGroupFromString(String s, Character keywordSe | |||
return newGroup; | |||
} | |||
|
|||
private static SmartGroup smartGroupFromString(String input, Character keywordSeparator) throws ParseException { | |||
if (!input.startsWith(MetadataSerializationConfiguration.SMART_GROUP_ID)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using exceptions for control flow is not recommended. Instead of throwing an IllegalArgumentException, consider using a different approach to handle this condition.
@@ -26,6 +27,19 @@ private static String serializeAllEntriesGroup() { | |||
return MetadataSerializationConfiguration.ALL_ENTRIES_GROUP_ID; | |||
} | |||
|
|||
private String serializeSmartGroup(SmartGroup group) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method serializeSmartGroup is private and returns a String. According to the special instructions, new public methods should not return null and should use java.util.Optional. Consider making this method public and returning Optional.
@@ -206,6 +210,24 @@ private static KeywordGroup keywordGroupFromString(String s, Character keywordSe | |||
return newGroup; | |||
} | |||
|
|||
private static SmartGroup smartGroupFromString(String input, Character keywordSeparator) throws ParseException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method 'smartGroupFromString' throws a ParseException for normal control flow when parsing context fails. Exceptions should be used for exceptional states, not for normal control flow.
Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. You can see which checks are failing by locating the box "Some checks were not successful" on the pull request page. To see the test output, locate "Tests / Checkstyle (pull_request)" and click on it. In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues, commit, and push. |
void serializeSmartGroup() { | ||
SmartGroup group = new SmartGroup("mySmartGroup", GroupHierarchyType.INDEPENDENT, ','); | ||
List<String> serialization = groupSerializer.serializeTree(GroupTreeNode.fromGroup(group)); | ||
assertEquals(Collections.singletonList("0 SmartGroup:mySmartGroup;0;1;;;;"), serialization); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In JabRef, to create an empty list we use List.of() instead of Collections.emptyList(). This applies to creating single-element lists as well, where List.of() should be preferred for consistency.
void serializeSmartGroup() { | ||
SmartGroup group = new SmartGroup("mySmartGroup", GroupHierarchyType.INDEPENDENT, ','); | ||
List<String> serialization = groupSerializer.serializeTree(GroupTreeNode.fromGroup(group)); | ||
assertEquals(Collections.singletonList("0 SmartGroup:mySmartGroup;0;1;;;;"), serialization); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Collections.singletonList is less modern compared to List.of, which is preferred for creating immutable lists in Java.
Closes #12548
This PR introduce feature to add new group "Imported Entries"
To Do
Done
Imported entries
group in preferencesImport entries
group in library preferences0 SmartGroup:Import entries\
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)